src,tools: use template literals #5778

Trott · 2016-03-18T05:10:58Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

src, tools

Description of change

Convert string concatenation to template literals. Enforce with lint
rule.

This came out of the conversation around #5762. If this is not objectionable, then the idea would be to enable the prefer-template rule on other parts of the codebase, moving it incrementally to using template literals instead of string concatenation.

cjihrig · 2016-03-18T14:38:56Z

LGTM

targos · 2016-03-18T14:53:38Z

src/.eslintrc

+
+rules:
+  # suggest using template literals instead of string concatenation
+  prefer-template: 2


Missing new line at EOF

Rebased, added newline per @targos, and force pushed.

targos · 2016-03-18T14:54:35Z

One nit, but LGTM if CI is happy

jasnell · 2016-03-18T17:44:23Z

LGTM

williamkapke · 2016-03-19T05:40:29Z

Many of these are only doing the string templating at the beginning or at the end. Maybe this has changed and I missed it- but I thought doing concatenation was faster.

I admit- these paths are probably not hit too much, but it seems that ALL thing performance get nits ;)

benjamingr · 2016-03-19T13:07:31Z

None of the paths are hot and I think template strings except where justified specifically by perf makes sense. LGTM and +1

Convert string concatenation to template literals. Enforce with lint rule.

Trott · 2016-03-19T20:11:13Z

@williamkapke wrote:

I thought doing concatenation was faster

With the caveat that I may simply be doing the benchmark wrong, my tests are showing that template literals are faster.

Here's the code. (You'll have to npm install benchmark to get the Benchmark.js module.)

'use strict';

const Benchmark = require('benchmark');
const suite = new Benchmark.Suite;

const conclusion = 'and made quite a dish!';

suite.add('string concatenation', () => {
  `The fish was delish, ${conclusion}` ===
    'The fish was delish, and made quite a dish!';
});

suite.add('template literal', () => {
    'The fish was delish, ' + conclusion ===
      'The fish was delish, and made quite a dish!';
});

suite.on('cycle', function(event) {
  console.log(String(event.target));
});

suite.run();

And here are the results I'm getting with Node.js v5.9.0:

string concatenation x 6,529,708 ops/sec ±1.23% (87 runs sampled)
template literal x 7,726,309 ops/sec ±1.08% (88 runs sampled)

Not that it really matters because, as @benjamingr points out, none of this is in a hot path.

but it seems that ALL thing performance get nits

That has not been my experience. On the contrary, PRs with micro-optimizations on non-hot-path code and with no benchmark showing effectiveness are usually rejected on the grounds that they cause churn and reduce code readability for no measurable benefit.

targos · 2016-03-19T20:21:19Z

Template strings should be equally fast. They are desugared to string concatenation by V8's parser

jbergstroem · 2016-03-20T10:49:45Z

LGTM

benjamingr · 2016-03-20T12:22:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1975

benjamingr · 2016-03-20T13:02:57Z

Cheers f158a86

Convert string concatenation to template literals. Enforce with lint rule. PR-URL: #5778 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

williamkapke · 2016-03-21T07:56:21Z

@Trott @targos Super awesome to know. thanks++

Trott · 2016-03-21T20:38:39Z

Heh, whoops, looks like I flipped the labels in my test. What I have listed for string concatenation was actually for template literals and vice versa. String concatenation seems to be somewhere between 9% and 18% faster, at least on the machines I have available to test on, and using Node.js 5.9.0. Not enough to worry about for things that only happen once, but probably enough to think twice about extreme hot-path code.

Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing nodejs#5778 for it. PR-URL: nodejs#5809 Reviewed-By: James M Snell <jasnell@gmail.com>

Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing #5778 for it. PR-URL: #5809 Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 · 2016-03-22T16:47:00Z

This rule will be completely invalid as of #5103

benjamingr · 2016-03-22T16:52:22Z

@Fishrock123 mind elaborating? Which rule?

Trott · 2016-03-22T18:26:31Z

@Fishrock123 I checked out your cleanup-node.js branch, rebased against current master (which has this lint rule), and ran make jslint. It found no errors. I'm not sure what you mean by the rule being "completely invalid". My guess was "This PR is going to violate the lint rule" but that does not appear to be the case.

Convert string concatenation to template literals. Enforce with lint rule. PR-URL: #5778 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Conflicts: src/.eslintrc src/node.js

Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing #5778 for it. PR-URL: #5809 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2016-03-30T19:18:16Z

I'm not 100% convinced this should be backported. @Trott it isn't landing cleanly, would you be willing to manually backport, do you think it is necessary?

Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing #5778 for it. PR-URL: #5809 Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2016-03-30T19:53:29Z

Willing to: Sure.

Necessary: Meh, could go either way.

Here's the PR if you want it: #5960

(If you're not going to use it, just go ahead and close it.)

MylesBorins · 2016-03-30T19:58:20Z

thanks @Trott

Prefer the use of template string literals over string concatenation in the dns module, makes dns consistent with other modules basically doing #5778 for it. PR-URL: #5809 Reviewed-By: James M Snell <jasnell@gmail.com>

Convert string concatenation to template literals. Enforce with lint rule. PR-URL: nodejs#5778 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Convert string concatenation to template literals. Enforce with lint rule. PR-URL: #5778 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Trott added tools Issues and PRs related to the tools directory. lts-watch-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 18, 2016

Trott mentioned this pull request Mar 18, 2016

dns: refactor dns to more modern JavaScript #5762

Closed

targos reviewed Mar 18, 2016
View reviewed changes

src,tools: use template literals

92a0d69

Convert string concatenation to template literals. Enforce with lint rule.

Trott force-pushed the prefer-template branch from 9072d89 to 92a0d69 Compare March 19, 2016 19:37

benjamingr closed this Mar 20, 2016

benjamingr mentioned this pull request Mar 20, 2016

dns: use template literals #5809

Closed

Fishrock123 mentioned this pull request Mar 22, 2016

v5.9.1 Release Proposal #5831

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

MylesBorins mentioned this pull request Apr 11, 2016

V4.4.3 proposal #6149

Merged

Trott deleted the prefer-template branch January 13, 2022 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,tools: use template literals #5778

src,tools: use template literals #5778

Trott commented Mar 18, 2016

cjihrig commented Mar 18, 2016

targos Mar 18, 2016

Trott Mar 19, 2016

targos commented Mar 18, 2016

jasnell commented Mar 18, 2016

williamkapke commented Mar 19, 2016

benjamingr commented Mar 19, 2016

Trott commented Mar 19, 2016

targos commented Mar 19, 2016

jbergstroem commented Mar 20, 2016

benjamingr commented Mar 20, 2016

benjamingr commented Mar 20, 2016

williamkapke commented Mar 21, 2016

Trott commented Mar 21, 2016

Fishrock123 commented Mar 22, 2016

benjamingr commented Mar 22, 2016

Trott commented Mar 22, 2016

MylesBorins commented Mar 30, 2016

Trott commented Mar 30, 2016

MylesBorins commented Mar 30, 2016

src,tools: use template literals #5778

src,tools: use template literals #5778

Conversation

Trott commented Mar 18, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

cjihrig commented Mar 18, 2016

targos Mar 18, 2016

Choose a reason for hiding this comment

Trott Mar 19, 2016

Choose a reason for hiding this comment

targos commented Mar 18, 2016

jasnell commented Mar 18, 2016

williamkapke commented Mar 19, 2016

benjamingr commented Mar 19, 2016

Trott commented Mar 19, 2016

targos commented Mar 19, 2016

jbergstroem commented Mar 20, 2016

benjamingr commented Mar 20, 2016

benjamingr commented Mar 20, 2016

williamkapke commented Mar 21, 2016

Trott commented Mar 21, 2016

Fishrock123 commented Mar 22, 2016

benjamingr commented Mar 22, 2016

Trott commented Mar 22, 2016

MylesBorins commented Mar 30, 2016

Trott commented Mar 30, 2016

MylesBorins commented Mar 30, 2016